Introduce interactive signing state flags for funded states. #3637

dunxen · 2025-03-03T18:50:23Z

This PR includes some deferred follow-ups extracted from #3423 and introduces new state flags to track interactive signing along with persistence of the minimum information needed from a signing session to reconstruct it.

A top-level state flag was avoided so that this work is compatible with splicing as well as V2 channel establishment (dual-funding).

ldk-reviews-bot · 2025-03-03T18:50:25Z

👋 Thanks for assigning @wpaulino as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

TheBlueMatt

Did you want to include test coverage for restarts here?

lightning/src/ln/channel.rs

Cargo.toml

lightning/src/ln/interactivetxs.rs

dunxen · 2025-03-04T15:29:10Z

Did you want to include test coverage for restarts here?

Not yet. Tracked in #3636. Will need to be able to contribute inputs first to test a useful order of message exchange + restart.

lightning/src/ln/channel.rs

codecov · 2025-03-05T10:13:38Z

Codecov Report

Attention: Patch coverage is 56.81818% with 76 lines in your changes missing coverage. Please review.

Project coverage is 88.99%. Comparing base (c4d23bc) to head (aaaeb22).

Files with missing lines	Patch %	Lines
lightning/src/ln/channel.rs	56.09%	50 Missing and 4 partials ⚠️
lightning/src/ln/channelmanager.rs	45.45%	8 Missing and 4 partials ⚠️
lightning/src/ln/interactivetxs.rs	46.66%	8 Missing ⚠️
lightning/src/ln/dual_funding_tests.rs	87.50%	1 Missing ⚠️
lightning/src/util/ser.rs	85.71%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3637      +/-   ##
==========================================
- Coverage   89.04%   88.99%   -0.05%     
==========================================
  Files         155      155              
  Lines      122019   122116      +97     
  Branches   122019   122116      +97     
==========================================
+ Hits       108652   108683      +31     
- Misses      10701    10764      +63     
- Partials     2666     2669       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

wpaulino · 2025-03-05T21:15:29Z

@dunxen re-request when this is ready for review again, feel free to squash as well

ldk-reviews-bot · 2025-03-06T12:44:38Z

🔔 1st Reminder

Hey @TheBlueMatt @jkczyz @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

lightning/src/ln/channel.rs

wpaulino · 2025-03-06T16:39:56Z

lightning/src/ln/channel.rs

@@ -6924,11 +6933,72 @@ impl<SP: Deref> FundedChannel<SP> where
 				log_debug!(logger, "Reconnected channel {} with no loss", &self.context.channel_id());
 			}

+			// if next_funding_txid is set:


If possible, would be nice to get some test coverage in now for this

Might be tricky at the moment, but makes sense to try do that now 👍

Still tricky, but working on this now currently. I'll let you know if we'll have to defer this to a PR after "create outbound dual-funded channel" is complete.

lightning/src/ln/channelmanager.rs

lightning/src/ln/interactivetxs.rs

TheBlueMatt · 2025-03-06T23:01:54Z

Taking myself off since @wpaulino and @jkczyz are on this one. Aside from my first comment I don't have any more high-level feedback.

lightning/src/ln/channel.rs

wpaulino · 2025-04-01T00:47:17Z

lightning/src/ln/channel.rs

 		where L::Target: Logger
 	{
-		if !matches!(self.context.channel_state, ChannelState::AwaitingChannelReady(_)) {
+		if !matches!(self.context.channel_state, ChannelState::FundingNegotiated(flags) if flags.is_interactive_signing()) {


Should this also check if they already sent it? Unclear if the spec is fine with retransmitting even if we didn't ask for it.

What if they sent this first but we were supposed to? Should we disconnect/fail?

This isn't clear in the spec if we should disconnect/fail.

So clarification on the spec: since we are always meant to immediately (ASAP) send tx_signatures after receiving the remote initial commitment signed if we are up first, we do not care if the remote incorrectly sends tx_signatures "first". We must carry on with the protocol (i.e. make sure our tx_signatures gets sent eventually).

Cool. This should probably ignore any duplicate tx_signatures messages so we don't close the channel if they resend as part of reestablish. I think they may also resend it as part of reestablish even when we're no longer in FundingNegotiated.

lightning/src/ln/channel.rs

ldk-reviews-bot · 2025-04-05T11:21:52Z

🔔 1st Reminder

Hey @jkczyz @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-04-05T11:21:53Z

🔔 1st Reminder

Hey @jkczyz @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-04-07T11:22:20Z

🔔 2nd Reminder

Hey @jkczyz @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

ldk-reviews-bot · 2025-04-07T11:22:21Z

🔔 2nd Reminder

Hey @jkczyz @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

wpaulino

Feel free to squash

wpaulino · 2025-04-08T00:06:40Z

lightning/src/ln/channel.rs

-								update_fail_malformed_htlcs: vec![],
-								update_fee: None,
-							})
+							self.get_last_commitment_update_for_send(logger).ok()


Maybe we should drop this commit for now, and revisit splicing support as a follow-up cc @jkczyz. maybe_get_next_funding_txid will have to look at the pending FundingScopes for splices/RBF, and get_last_commitment_update_for_send now returns a batch of commitment_signeds when this should just send the one for the FundingScope in question.

wpaulino · 2025-04-08T00:10:07Z

lightning/src/ln/channel.rs

+				// if it also sets `next_funding_txid` in its own `channel_reestablish`, but the values don't match:
+				if let Some(our_next_funding_txid) = self.maybe_get_next_funding_txid().filter(|txid| txid != &next_funding_txid) {
+					//   MUST send an `error` and fail the channel.
+					return Err(ChannelError::Close((format!(


What's the actual requirement? We seem to have two conflicting ones: here we fail if there's a mismatch, but below we send TxAbort instead.

It's in the current splicing PR. This one is fail and below is tx_abort:

- if it also sets `next_funding_txid` in its own `channel_reestablish`, but the values don't match: - MUST send an `error` and fail the channel. - otherwise: - MUST send `tx_abort` to let the sending node know that they can forget this funding transaction.

https://github.com/lightning/bolts/blob/4e82adcd848466b36a6d8a717b195eabe24de216/02-peer-protocol.md?plain=1#L3338

lightning/src/ln/channel.rs

jkczyz

Looks like there's some breakages in the full_stack fuzz test.

lightning/src/ln/channel.rs

dunxen · 2025-04-09T08:58:30Z

Looks like there's some breakages in the full_stack fuzz test.

Thanks, will address those. Honestly haven't paid much attention as fuzz had been broken for a while on main I believe.

Instead of having an explicit `ChannelContext::next_funding_txid` to set and read, we can get this value on the fly when it is appropriate to do so.

This follows the the specification closely in branching without being too verbose, so that it should be easy to follow the logic. See: https://github.com/lightning/bolts/blob/aa5207a/02-peer-protocol.md?plain=1#L2520-L2531

jkczyz · 2025-04-09T13:47:16Z

Hmm... when possible please avoid rebasing unless necessary. Otherwise, it makes it difficult to view new changes related to the PR. If needed, doing two separate pushes would be ideal.

jkczyz · 2025-04-09T13:48:25Z

Thanks, will address those. Honestly haven't paid much attention as fuzz had been broken for a while on main I believe.

Locally, it still should work. CI has just been timing out (unfortunately) but will catch actual failures, AFAICT.

dunxen · 2025-04-09T13:51:14Z

Locally, it still should work. CI has just been timing out (unfortunately) but will catch actual failures, AFAICT.

Oh yeah I see this was just related to the new bool in config.

dunxen · 2025-04-09T13:52:35Z

Hmm... when possible please avoid rebasing unless necessary.

Sorry, bad habit I do sometimes. I'll keep it in mind again.

This intoduces the INTERACTIVE_SIGNING, THEIR_TX_SIGNATURES_SENT, and OUR_TX_SIGNATURES_SENT funded state flags. A top-level state flag for INTERACTIVE_SIGNING was avoided so that this work is compatible with splicing as well as V2 channel establishment (dual-funding).

We fully persist `InteractiveTxSigningSession` as it provides the full context of the constructed transaction which is still needed for signing.

When this config field is enabled, the dual_fund feature bit will be set which determines support when receiving `open_channel2` messages.

wpaulino · 2025-04-10T00:10:28Z

lightning/src/ln/channel.rs

@@ -5809,7 +5848,6 @@ impl<SP: Deref> FundedChannel<SP> where
 		log_info!(logger, "Received initial commitment_signed from peer for channel {}", &self.context.channel_id());

 		let need_channel_ready = self.check_get_channel_ready(0, logger).is_some();
-		self.context.channel_state = ChannelState::AwaitingChannelReady(AwaitingChannelReadyFlags::new());
 		self.monitor_updating_paused(false, false, need_channel_ready, Vec::new(), Vec::new(), Vec::new());


I don't think we need to check for channel ready here because it should be impossible to send splice_locked if tx_signatures has not been exchanged. It does mean we'll have to check it once we receive tx_signatures though.

wpaulino · 2025-04-10T00:15:41Z

lightning/src/ln/channel.rs

 		where L::Target: Logger
 	{
-		if !matches!(self.context.channel_state, ChannelState::AwaitingChannelReady(_)) {
+		if !matches!(self.context.channel_state, ChannelState::FundingNegotiated(flags) if flags.is_interactive_signing()) {


Cool. This should probably ignore any duplicate tx_signatures messages so we don't close the channel if they resend as part of reestablish. I think they may also resend it as part of reestablish even when we're no longer in FundingNegotiated.

dunxen marked this pull request as draft March 3, 2025 18:50

dunxen mentioned this pull request Dec 5, 2024

Dual-funded channels and Splicing Project Tracking #1621

Open

7 tasks

dunxen force-pushed the 2025-02-interactivesigningstate branch 2 times, most recently from 4c6b6ab to c1f430a Compare March 4, 2025 09:02

dunxen changed the title ~~DRAFT: Introduce interactive signing state flags for funded states.~~ Introduce interactive signing state flags for funded states. Mar 4, 2025

dunxen marked this pull request as ready for review March 4, 2025 09:03

dunxen requested review from TheBlueMatt, jkczyz and wpaulino March 4, 2025 09:04

dunxen force-pushed the 2025-02-interactivesigningstate branch from c1f430a to e89ba58 Compare March 4, 2025 11:09

TheBlueMatt reviewed Mar 4, 2025

View reviewed changes

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

Cargo.toml Outdated Show resolved Hide resolved

lightning/src/ln/interactivetxs.rs Outdated Show resolved Hide resolved

wpaulino reviewed Mar 4, 2025

View reviewed changes

dunxen force-pushed the 2025-02-interactivesigningstate branch from e89ba58 to 3b2ac55 Compare March 5, 2025 10:05

dunxen force-pushed the 2025-02-interactivesigningstate branch from 3b2ac55 to 5110ecc Compare March 5, 2025 14:20

dunxen force-pushed the 2025-02-interactivesigningstate branch from 5110ecc to 1d96044 Compare March 6, 2025 12:43

dunxen requested review from TheBlueMatt and wpaulino March 6, 2025 12:44

dunxen added weekly goal Someone wants to land this this week Dual-funding labels Mar 6, 2025

wpaulino reviewed Mar 6, 2025

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

jkczyz reviewed Mar 6, 2025

View reviewed changes

lightning/src/ln/interactivetxs.rs Outdated Show resolved Hide resolved

TheBlueMatt removed their request for review March 6, 2025 23:01

dunxen force-pushed the 2025-02-interactivesigningstate branch from 1d96044 to 55e5f6f Compare March 11, 2025 11:42

wpaulino reviewed Apr 1, 2025

View reviewed changes

dunxen force-pushed the 2025-02-interactivesigningstate branch 2 times, most recently from 5291edc to c99182e Compare April 3, 2025 11:10

dunxen requested review from jkczyz and wpaulino April 3, 2025 11:20

dunxen force-pushed the 2025-02-interactivesigningstate branch from c99182e to aaaeb22 Compare April 7, 2025 08:09

dunxen force-pushed the 2025-02-interactivesigningstate branch from aaaeb22 to 206faf0 Compare April 7, 2025 11:27

wpaulino reviewed Apr 8, 2025

View reviewed changes

dunxen force-pushed the 2025-02-interactivesigningstate branch from 206faf0 to d9a8527 Compare April 8, 2025 11:38

jkczyz reviewed Apr 8, 2025

View reviewed changes

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

wpaulino reviewed Apr 9, 2025

View reviewed changes

lightning/src/ln/channel.rs Show resolved Hide resolved

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

dunxen added 2 commits April 9, 2025 11:14

Get next_funding_txid from the funding_outpoint based on state

1b2ae3b

Instead of having an explicit `ChannelContext::next_funding_txid` to set and read, we can get this value on the fly when it is appropriate to do so.

Handle receiving channel_reestablish with next_funding_txid

492499a

This follows the the specification closely in branching without being too verbose, so that it should be easy to follow the logic. See: https://github.com/lightning/bolts/blob/aa5207a/02-peer-protocol.md?plain=1#L2520-L2531

dunxen force-pushed the 2025-02-interactivesigningstate branch from d9a8527 to aec4e43 Compare April 9, 2025 13:31

dunxen requested review from jkczyz and wpaulino April 9, 2025 13:33

dunxen added 3 commits April 9, 2025 17:02

Persist InteractiveTxSigningSession to resume signing across restarts

f644e99

We fully persist `InteractiveTxSigningSession` as it provides the full context of the constructed transaction which is still needed for signing.

Add enable_dual_funded_channels to UserConfig

4f6192f

When this config field is enabled, the dual_fund feature bit will be set which determines support when receiving `open_channel2` messages.

dunxen force-pushed the 2025-02-interactivesigningstate branch from aec4e43 to 4f6192f Compare April 9, 2025 15:02

wpaulino reviewed Apr 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce interactive signing state flags for funded states. #3637

Introduce interactive signing state flags for funded states. #3637

dunxen commented Mar 3, 2025

ldk-reviews-bot commented Mar 3, 2025 •

edited

Loading

TheBlueMatt left a comment

dunxen commented Mar 4, 2025

codecov bot commented Mar 5, 2025 •

edited

Loading

wpaulino commented Mar 5, 2025

ldk-reviews-bot commented Mar 6, 2025

wpaulino Mar 6, 2025

dunxen Mar 7, 2025

dunxen Mar 31, 2025

TheBlueMatt commented Mar 6, 2025

wpaulino Apr 1, 2025

wpaulino Apr 1, 2025

dunxen Apr 2, 2025

dunxen Apr 9, 2025

wpaulino Apr 10, 2025

ldk-reviews-bot commented Apr 5, 2025

ldk-reviews-bot commented Apr 5, 2025

ldk-reviews-bot commented Apr 7, 2025

ldk-reviews-bot commented Apr 7, 2025

wpaulino left a comment

wpaulino Apr 8, 2025 •

edited

Loading

wpaulino Apr 8, 2025

dunxen Apr 8, 2025

jkczyz left a comment

dunxen commented Apr 9, 2025

jkczyz commented Apr 9, 2025

jkczyz commented Apr 9, 2025

dunxen commented Apr 9, 2025

dunxen commented Apr 9, 2025

wpaulino Apr 10, 2025

wpaulino Apr 10, 2025

Introduce interactive signing state flags for funded states. #3637

Are you sure you want to change the base?

Introduce interactive signing state flags for funded states. #3637

Conversation

dunxen commented Mar 3, 2025

ldk-reviews-bot commented Mar 3, 2025 • edited Loading

TheBlueMatt left a comment

Choose a reason for hiding this comment

dunxen commented Mar 4, 2025

codecov bot commented Mar 5, 2025 • edited Loading

Codecov Report

wpaulino commented Mar 5, 2025

ldk-reviews-bot commented Mar 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Mar 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldk-reviews-bot commented Apr 5, 2025

ldk-reviews-bot commented Apr 5, 2025

ldk-reviews-bot commented Apr 7, 2025

ldk-reviews-bot commented Apr 7, 2025

wpaulino left a comment

Choose a reason for hiding this comment

wpaulino Apr 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz left a comment

Choose a reason for hiding this comment

dunxen commented Apr 9, 2025

jkczyz commented Apr 9, 2025

jkczyz commented Apr 9, 2025

dunxen commented Apr 9, 2025

dunxen commented Apr 9, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldk-reviews-bot commented Mar 3, 2025 •

edited

Loading

codecov bot commented Mar 5, 2025 •

edited

Loading

wpaulino Apr 8, 2025 •

edited

Loading